@llvm/pr-subscribers-backend-spir-v

Author: Juan Manuel Martinez Caamaño (jmmartinez)

; SPIR-V
; Version: 1.0
; Generator: Khronos LLVM/SPIR-V Translator; 14
; Bound: 11
; Schema: 0
               OpCapability Addresses
               OpCapability Linkage
               OpCapability Kernel
          %1 = OpExtInstImport "OpenCL.std"
               OpMemoryModel Physical64 OpenCL
               OpEntryPoint Kernel %8 "foo"
               OpExecutionMode %8 ContractionOff
               OpSource Unknown 0
               OpName %bar "bar"
               OpName %foo "foo"
               OpName %entry "entry"
               OpDecorate %bar LinkageAttributes "bar" Import
               OpDecorate %foo LinkageAttributes "foo" Export
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
        %bar = OpFunction %void None %3
               OpFunctionEnd
        %foo = OpFunction %void None %3
      %entry = OpLabel
          %7 = OpFunctionCall %void %bar
               OpReturn
               OpFunctionEnd
          %8 = OpFunction %void None %3
          %9 = OpLabel
         %10 = OpFunctionCall %void %foo
               OpReturn
               OpFunctionEnd

diff --git a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
index 4e2cc882ed6ba..b4b2c8d5947ce 100644
--- a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
@@ -1042,7 +1042,7 @@ getFirstValidInstructionInsertPoint(MachineBasicBlock &BB) {
 
 std::optional<SPIRV::LinkageType::LinkageType>
 getSpirvLinkageTypeFor(const SPIRVSubtarget &ST, const GlobalValue &GV) {
-  if (GV.hasLocalLinkage() || GV.hasHiddenVisibility())
+  if (GV.hasLocalLinkage())
     return std::nullopt;
 
   if (GV.isDeclarationForLinker())
diff --git a/llvm/test/CodeGen/SPIRV/linkage/link-hidden.ll b/llvm/test/CodeGen/SPIRV/linkage/link-hidden.ll
new file mode 100644
index 0000000000000..f4172df38ee8b
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/linkage/link-hidden.ll
@@ -0,0 +1,14 @@
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val %}
+
+; CHECK: OpName %[[BAR:[0-9]+]] "bar"
+; CHECK: OpDecorate %[[BAR]] LinkageAttributes "bar" Import
+; CHECK: %[[BAR]] = OpFunction
+
+define hidden spir_kernel void @foo() addrspace(4) {
+entry:
+  call spir_func addrspace(4) void @bar()
+  ret void
+}
+
+declare hidden spir_func void @bar() addrspace(4)

I think that there is a change to reduce the scope of the change, to only certain well known functions. My first proposal is to match the translator's behavior.

I missed one error in link-attribute-vk.ll. I'll check that.

update:
Arf... its coming from #144701 where we explicitly want hidden not to emit decorations for the kind of variable in that llvm-ir.

update:
It seems to me that this patch was done to handle this particular type of builtin variable.

I wonder if discriminating using the spirv.Decorations metadata would be enough to cover this cases (these variable have the spirv.Decorations metadata set with {i32 11 (builtin), i32 <builtin-id>}). @Keenuts Would this be enough to cover the hlsh use-case?

As a reference, the translator still adds the builtin variable to the imports:

OpDecorate %sv_position LinkageAttributes "sv_position" Import
OpDecorate %sv_position Constant
OpDecorate %sv_position BuiltIn Position

This might not work well for HLSL as is. See https://github.com/llvm/wg-hlsl/blob/main/proposals/0026-symbol-visibility.md.

"hidden" visibility is different from "internal/private" linkage types. A global variable or function with hidden public linkage should still be visible to other compilation units in the same "shared object".

It is also different from something with default visibility. Something will be "wrong" because, as you noted, SPIR-V cannot make that distinction.

Some of the difficulties with this is that SPIR-V does not have a concept of a shared object. We have modules. Spirv-link can link modules. For HLSL, we took the view that a module is a shared object, and you could link the shared object. This is because there is a long term plan to have linking done at the llvm-ir level. So the individual llvm-ir files are a translation unit, but they are "linked" before entering the BE to be a shared library. We could add an "HLSL linking pass" before reaching the backend that will change the linkage of the symbols, however, to be accepted we might need to do a full linking tool.

Can you write up a design doc on how you want the linkages to work for OpenCL? Also, do you know which language constructs result in hidden visibility? We should be able to find a solution that works for everyone.

This might not work well for HLSL as is. See https://github.com/llvm/wg-hlsl/blob/main/proposals/0026-symbol-visibility.md.

Thanks for the doc ! Yeah, this PR goes against this. I'll close this PR for the moment and come back later with more background. There is probably another way to get to the same place that I don't know about yet.

Thanks for the review !